Introduce RoundingIncrement to FixedDecimal
#4219
Conversation
jedel1043
force-pushed
the
rounding-increments
branch
2 times, most recently
from
6965970
to
e96d4f2
Compare
|
Will implement the FFI glue code until the API is accepted, but everything else should be fine. |
jedel1043
force-pushed
the
rounding-increments
branch
from
dba796b
to
d612b43
Compare
jedel1043
changed the title
RoundingIncrement to FixedDecimalRoundingCoarseness to FixedDecimal
jedel1043
force-pushed
the
rounding-increments
branch
from
f5c7b15
to
ff854eb
Compare
utils/fixed_decimal/src/decimal.rs
Outdated
| /// dec.trunc_coarse(-2, RoundingCoarseness::MultiplesOf2); | ||
| /// assert_eq!("9.98", dec.to_string()); | ||
| /// ``` | ||
| pub fn trunc_coarse(&mut self, position: i16, coarseness: RoundingCoarseness) { |
There was a problem hiding this comment.
naming: using "truncation" and "rounding" in the same API is confusing. This API never rounds, so RoundingCoarseness should probably be TruncationCoarseness, or some neutral term.
There was a problem hiding this comment.
Though, ECMA402 considers truncation as a "roundingMode"...
The obvious problem would be that TruncationCoarseness has the same problem when used in expand_coarse, since the API never truncates, and I can't find another term to encompass all operations.
Maybe we can just leave it as Coarseness?
There was a problem hiding this comment.
I guess truncation is a special case of rounding. We can bikeshed this later.
sffc
added
discuss-priority
|
Renaming to |
jedel1043
force-pushed
the
rounding-increments
branch
from
3c93c43
to
a474015
Compare
jedel1043
changed the title
RoundingCoarseness to FixedDecimalRoundingIncrement to FixedDecimal
jedel1043
force-pushed
the
rounding-increments
branch
from
a474015
to
49ad0bb
Compare
jedel1043
force-pushed
the
rounding-increments
branch
from
49ad0bb
to
08032c4
Compare
utils/fixed_decimal/src/decimal.rs
Outdated
| } | ||
| RoundingIncrement::MultiplesOf2 => { | ||
| let Some(last_digit) = self.digits.last_mut() else { | ||
| // Unreachable |
There was a problem hiding this comment.
Nit: use debug_assert!(false, "message") for unreachable code.
|
|
||
| // Equivalent to (n / 2) * 2, which truncates to the previous | ||
| // multiple of two | ||
| *last_digit &= 0xFE; |
There was a problem hiding this comment.
Praise: this is a nice way to truncate to multiple of 2
| // Extend with zeroes to have the correct trailing digits. | ||
| self.digits.resize(digits_to_retain as usize, 0); |
There was a problem hiding this comment.
Question: what does this do? You already truncated self.digits to length digits_to_retain up above.
There was a problem hiding this comment.
This is for cases such as "0.60", which is represented as digits = [6], magnitude = -1.
Since we need digits = [6, 0] instead to modify to the next increment of 25, we extend to the exact digits of digits_to_retain so that we can modify the last, nonexistent zero.
There was a problem hiding this comment.
I see, okay. "0.3" should truncate down to "0.25" which requires adding an extra digit, and this digit is reflected in digits_to_retain.
This PR implements an MVP of the
RoundingIncrementAPI forFixedDecimal, which should allow rounding to increments that are divisors of 10 and 100. It also implementsFixedDecimal::trunc_to_incrementto show the usage of the API.I plan to implement the remaining methods, but I wanted to create an MVP first to discuss if this would be the desired API.
Mostly based from #3929 (comment)